Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Only override process.binding('fs') #182

Merged
merged 4 commits into from
Dec 8, 2016
Merged

Only override process.binding('fs') #182

merged 4 commits into from
Dec 8, 2016

Conversation

tschaub
Copy link
Owner

@tschaub tschaub commented Nov 1, 2016 •
edited
Loading

This reworks how the fs module is mocked. Instead of overriding methods on the module itself, only the process.binding('fs') methods are replaced.

Possible breaking changes:

  • The mock.fs() function has been removed (this could be added back)
  • The object created by fs.Stats is no longer an instanceof fs.Stats (this could be possible to support again)
  • Lazy require() calls don't work consistently (perhaps this can be made to work again too)
  • No more support for Node < 4 (tests are not run there, but things probably still work)

@tschaub tschaub mentioned this pull request Nov 1, 2016
Merged
not-an-aardvark
not-an-aardvark reviewed Nov 1, 2016
View reviewed changes
Copy link
Contributor

@not-an-aardvark not-an-aardvark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, this looks like a very nice simplification if it works.

It's probably worth noting that process.binding('fs') isn't officially a stable Node feature; it's possible that the binding will unexpectedly change in the future, or do something different between major Node versions. But it looks like you already have a lib/binding.js file, so I suppose that problem isn't newly introduced by this change.

@eugef
Copy link

eugef commented Nov 7, 2016

I am using Node 6.3 and Babel to transpile import statements.
The current master doesn't work for me.

But this PR works for me, great job @tschaub !

Any plans to release it?

@tschaub
Copy link
Owner Author

tschaub commented Nov 7, 2016

@not-an-aardvark - Yeah, I'm aware that process.binding('fs') is not a stable feature. It turns out that the mock-fs binding works with Node 0.8 through 7. So although there have been Node releases with breaking fs changes, I've been able to maintain a single binding that has worked with them all.

@eugef - I haven't had time to address the breaking changes mentioned above. I'm personally most interested in fixing the require issue (making require consistently work with the real fs even when called lazily). Not sure which of these will affect the most users.

I'll release a beta and see if we can get some feedback.

@tschaub tschaub changed the title (WIP) Only override process.binding('fs') Only override process.binding('fs') Dec 8, 2016
@tschaub tschaub merged commit 5c74f6f into master Dec 8, 2016
@tschaub tschaub deleted the binding branch December 8, 2016 15:36
@eugef
Copy link

eugef commented Dec 8, 2016

When it would be released?

@tschaub
Copy link
Owner Author

tschaub commented Dec 8, 2016

You can install mock-fs@beta to try it out.

@tschaub tschaub mentioned this pull request Nov 7, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@not-an-aardvark not-an-aardvark not-an-aardvark left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tschaub @eugef @not-an-aardvark